Repository navigation
fix(versions): pin the trunk to published releases and fix released pin headers - #710
Conversation
✅ Deploy Preview for cozystack ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
88c740b to
78a34c4
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM.
Labelled docs, but it's really pipeline work behind data/versions/*.yaml, and the accuracy checks out. I ran each claim against the tree. The jq lines added to CLAUDE.md and CONTRIBUTING.md are right: hack/download_openapi.sh calls jq unconditionally and runs in the Netlify production build, and make update-all with no RELEASE_TAG hits the jq-backed releases-API path in the Makefile. The promote-rc.yaml comment in release_next.sh is accurate too, since the upstream workflow regenerates next.yaml from the staging ref before release-next. v1.6.yaml is indeed the only released pin file still carrying the trunk header. The new snapshot logic reproduces the committed v1.6.yaml byte for byte, and hack/test_version_pins.sh is green end to end on GNU sed, 15 of 15 checks.
Two non-blocking notes below. Neither blocks the merge.
Findings
- [MINOR]
hack/test_version_pins.sh:95, Self-check is GNU-sed only; BSD sed aborts it
- [NIT]
hack/update_versions.sh:40,|| truemasks a jq parse failure
| # copying every value line except the release-coupled cozystack pins. | ||
| mkdir -p "$sandbox/hack" "$sandbox/data/versions" "$sandbox/content/en/docs/next" | ||
| cp hugo.yaml "$sandbox/" | ||
| cp hack/release_next.sh hack/register_version.sh "$sandbox/hack/" |
There was a problem hiding this comment.
[MINOR] Self-check is GNU-sed only; BSD sed aborts it
[MINOR] The final block copies register_version.sh into the sandbox and runs the whole release_next.sh, which calls it. register_version.sh:80 uses the GNU-only sed "/^ versions:$/a\\${BLOCK}" append, so on BSD sed (macOS) the run aborts with extra characters after \ before the snapshot checks ever execute. Reproduced here: under GNU sed all 15 checks pass, under BSD sed the script exits non-zero partway through. The root cause is pre-existing in register_version.sh, not this PR, and CI runs on Linux so it is green there. But the header says "Run from the repo root", so a one-line note that the self-check assumes GNU coreutils would save a confusing local failure for anyone running it on macOS.
There was a problem hiding this comment.
IvanHunters Reproduced it with /usr/bin/sed. When sed is not GNU, the self-check now puts gsed in front of it, and if gsed is missing it stops with a clear message, so it passes 16/16 under both. The sed calls in register_version.sh are filed as #731, because on BSD sed the update path also deletes hidden and label lines without any error.
| # by version, not creation time: a patch of an older minor can be the newest. | ||
| latest_published_tag() { | ||
| jq -r '.[] | select(.draft == false and .prerelease == false) | .tag_name' \ | ||
| | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' | sort -V | tail -1 || true |
There was a problem hiding this comment.
[NIT] || true masks a jq parse failure
[NIT] With set -euo pipefail, the trailing || true on jq | grep | sort | tail swallows a genuine jq failure (malformed JSON) as well as the intended empty result. When that happens the caller reports no published vX.Y.Z release found rather than a parse error. Harmless in practice, since the JSON comes from a curl -f'd GitHub API response, but attaching || true to tail alone, or checking jq separately, keeps the two failure modes distinct.
There was a problem hiding this comment.
|| true now covers only the empty grep result. A jq failure is returned to the caller, which reports it as a parse error, and there is a test with malformed JSON for it.
78a34c4 to
03a2c66
Compare
03a2c66 to
6c7c16c
Compare
0633781 to
18c92a3
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
Verified against head 18c92a3e vs merge-base 1c201210: the pin pipeline is correct, v1.6.yaml is byte-identical to a sandbox run of release_next.sh, and the upstream pins are accurate. Two non-blocking notes below.
Findings
- [MINOR]
hack/update_versions.sh:96, resolver reads only the newest 100 releases (per_page=100, no pagination)
- [MINOR]
hack/test_version_pins.sh:139, prerelease-skip assertion does not exercise the prerelease filter
Caveats
- Verified clean and not re-litigating:
v1.6.0is published (draft=false, prerelease=false, 17 assets);talos v1.13.6matchesinstaller.yamlat tagv1.6.0;release_next.shreproduces the committedv1.6.yamlbyte-for-byte; the token is sent via--header @-on stdin, never argv; the no-jq/no-tag path exits non-zero and leaves the pin file untouched;hack/test_version_pins.shpasses 16/16; the mechanical swallowed-error sweep found no decision gate that fails open.
Findings not anchored to changed lines
These reference code outside this PR's diff (unchanged files, or lines outside a hunk), so GitHub cannot render them inline.
[MINOR] hack/test_version_pins.sh:139 prerelease-skip assertion does not exercise the prerelease filter
The only prerelease fixture in "resolver skips a draft and a prerelease" is v1.7.0-rc.1, which grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' rejects on its own regardless of the select(... .prerelease == false) clause. Dropping that clause leaves the test green, so the prerelease dimension is uncovered (the draft dimension is covered, its fixture is a plain-semver v1.6.4). A plain-semver release marked prerelease is what the filter guards. Add a {"tag_name":"v1.6.5","draft":false,"prerelease":true} fixture and assert it is skipped.
| fi | ||
| # Optional token for the higher API rate limit. | ||
| token="${GITHUB_TOKEN:-${GH_TOKEN:-}}" | ||
| RELEASES_URL="https://github.com/ghapi/repos/${SOURCE_REPO}/releases?per_page=100" |
There was a problem hiding this comment.
[MINOR] resolver reads only the newest 100 releases (per_page=100, no pagination)
RELEASES_URL sets per_page=100 and fetch_releases never paginates, so latest_published_tag sees only the 100 most-recently-created releases. cozystack has 224 releases today and 23 of the newest 100 are prereleases, so a published final release that is the highest by version but was created more than 100 releases ago would be invisible and the resolver would pin a lower (still-published) version. Latent now, not live: v1.6.4 is both the highest version and recent, so page 1 resolves to it correctly. Either paginate (follow Link: rel="next") or document the 100-newest-releases bound in the function comment.
update_versions.sh picked the default cozystack_tag from the upstream git tag list. A tag exists as soon as it is pushed, while its GitHub release can still be a draft with no assets, so the trunk could be pinned to a version whose releases/download URLs all return 404. Resolve the default from the GitHub releases API instead, keeping only published, non-draft, non-prerelease releases and taking the highest version rather than the newest one, since a patch of an older minor can be created after a newer minor. An explicit --cozystack-tag still wins. An optional GITHUB_TOKEN goes to curl on stdin rather than in argv, so it never shows up in a process listing. Without a tag the script needs jq and says so when it is missing, instead of reporting that upstream has no published release. CLAUDE.md and CONTRIBUTING.md now list jq as a required tool. hack/test_version_pins.sh is an offline self-check that feeds the resolver release-list fixtures and runs the script against a stub curl to check where the token goes and the missing-jq error. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
release_next.sh copied next.yaml into the new vX.Y.yaml verbatim, so a released pin file kept the trunk header saying 'make update-all' regenerates it to track upstream main. That is false for a released file, which update-all never touches. Rewrite the leading comment block and the trunk wording in the section comments when snapshotting, and correct v1.6.yaml, the one released file that carried the trunk header. The header does not repeat the pinned version: patch releases bump the values by hand, and a copy in the comment would go stale. Only comments change; no pinned value moves. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
18c92a3 to
2f71593
Compare
Two fixes in the pipeline behind
data/versions/*.yaml.A plain
make update-allcould pin the trunk to a release that does not exist yet. With no--cozystack-tag,hack/update_versions.shtook the newest upstream git tag, but a tag exists as soon as it is pushed, while its GitHub release can still be a draft with no assets. That is how the trunk once ended up on v1.5.3 with everyreleases/downloadURL returning 404. The default now comes from the GitHub releases API: the highest published, non-draft, non-prerelease release. It sorts by version rather than creation time, because a patch of an older minor can be published after a newer minor. An explicit--cozystack-tagstill wins. If the API request fails or finds nothing published, the script exits with an error and leaves the pin file untouched.make update-allwithout a tag now needsjq, so the required-tools lists inCLAUDE.mdandCONTRIBUTING.mdboth name it.hack/release_next.shcopiednext.yamlinto the newvX.Y.yamlverbatim, so released files kept the trunk header sayingmake update-allregenerates them. The snapshot now gets its own header, andv1.6.yaml, the only released file that had the trunk header, is corrected. Only comments change, no pinned value moves.hack/test_version_pins.shis an offline self-check. It feeds the resolver release lists with a draft, a prerelease, an older-minor patch created last and nothing published, and runsrelease_next.shin a sandbox to check the header and values. Against the live API the script produces exactly the committednext.yaml.hugo --gc --minifypasses.